Add AAP provisioning and deprovisioning workflow - #4
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
WalkthroughThis PR integrates AAP-backed provisioning and deprovisioning workflows into the HostLeaseReconciler. It updates the toolchain (Go 1.25→1.26), replaces bare-metal-operator with bare-metal-fulfillment-operator, introduces AAP environment variable configuration, initializes the provisioning provider in main(), extends the reconciler to accept and use the provider, implements provisioning/deprovisioning job lifecycle management, refactors status persistence logic, and adds comprehensive test coverage. A sample HostLease configuration is updated to use the new provisioning template. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/hostlease_controller_test.go (1)
690-837: ⚡ Quick winAssert “no provisioning call” explicitly in skip-path tests.
These tests only assert
Reconcilereturns success; they don’t verify that provisioning APIs were not invoked. That can let regressions pass if a provider call happens but still returns nil. Use a test double forProvisioningProviderwith call counters and assert zero invocations in the nil/noop/empty/already-succeeded scenarios (and optionally assert jobs/conditions remain unchanged where applicable).As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/hostlease_controller_test.go` around lines 690 - 837, The tests in Describe("reconcileProvisioning") only assert Reconcile returns successfully but do not verify that the ProvisioningProvider was not invoked; add a test-double implementation of provisioning.AAPProvider (or an interface wrapper used by reconciler.ProvisioningProvider) with a call counter for the provision method(s) and inject it into each skip-path test (the nil/noop/empty-template/already-succeeded cases) and assert the counter is zero after reconciler.Reconcile returns; additionally for the already-succeeded case assert HostLease.Status.Jobs (and any relevant conditions) remain unchanged to ensure no new job was created.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/main.go`:
- Around line 238-247: When OSAC_AAP_URL (controller.EnvAAPURL) is set but
OSAC_AAP_TOKEN (controller.EnvAAPToken) is missing, modify the boot logic to
fail fast instead of constructing a broken aap client; check the value of
aapToken after reading it from the environment and return/log a fatal error
(stop startup) if it's empty. Update the block that constructs
aap.NewClient(...) and provisioning.NewProvider(...) so you validate aapToken
first and only call aap.NewClient and provisioning.NewProvider with a valid
token, ensuring the application exits with a clear error rather than continuing
with an invalid provisioning.ProviderConfig.
---
Nitpick comments:
In `@internal/controller/hostlease_controller_test.go`:
- Around line 690-837: The tests in Describe("reconcileProvisioning") only
assert Reconcile returns successfully but do not verify that the
ProvisioningProvider was not invoked; add a test-double implementation of
provisioning.AAPProvider (or an interface wrapper used by
reconciler.ProvisioningProvider) with a call counter for the provision method(s)
and inject it into each skip-path test (the
nil/noop/empty-template/already-succeeded cases) and assert the counter is zero
after reconciler.Reconcile returns; additionally for the already-succeeded case
assert HostLease.Status.Jobs (and any relevant conditions) remain unchanged to
ensure no new job was created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: f1799af5-f1c3-4624-8c77-15a8c12855da
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
Containerfilecmd/main.gocmd/main_test.goconfig/samples/v1alpha1_hostlease.yamlgo.modinternal/controller/hostlease_controller.gointernal/controller/hostlease_controller_test.gointernal/controller/hostlease_names.go
| if aapURL := os.Getenv(controller.EnvAAPURL); aapURL != "" { | ||
| aapToken := os.Getenv(controller.EnvAAPToken) | ||
| insecureSkipVerify, _ := strconv.ParseBool(os.Getenv(controller.EnvAAPInsecureSkipVerify)) | ||
| templatePrefix := os.Getenv(controller.EnvAAPTemplatePrefix) | ||
| if templatePrefix == "" { | ||
| templatePrefix = "osac" | ||
| } | ||
| aapClient := aap.NewClient(aapURL, aapToken, insecureSkipVerify) | ||
| var err error | ||
| provisioningProvider, err = provisioning.NewProvider(provisioning.ProviderConfig{ |
There was a problem hiding this comment.
Fail fast when AAP URL is configured but token is missing.
When OSAC_AAP_URL is set and OSAC_AAP_TOKEN is empty, the controller still boots with a broken provisioning provider and only fails later at runtime.
Suggested fix
if aapURL := os.Getenv(controller.EnvAAPURL); aapURL != "" {
aapToken := os.Getenv(controller.EnvAAPToken)
+ if aapToken == "" {
+ setupLog.Error(nil, fmt.Sprintf("%s must be set when %s is configured", controller.EnvAAPToken, controller.EnvAAPURL))
+ os.Exit(1)
+ }
insecureSkipVerify, _ := strconv.ParseBool(os.Getenv(controller.EnvAAPInsecureSkipVerify))
templatePrefix := os.Getenv(controller.EnvAAPTemplatePrefix)
if templatePrefix == "" {
templatePrefix = "osac"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if aapURL := os.Getenv(controller.EnvAAPURL); aapURL != "" { | |
| aapToken := os.Getenv(controller.EnvAAPToken) | |
| insecureSkipVerify, _ := strconv.ParseBool(os.Getenv(controller.EnvAAPInsecureSkipVerify)) | |
| templatePrefix := os.Getenv(controller.EnvAAPTemplatePrefix) | |
| if templatePrefix == "" { | |
| templatePrefix = "osac" | |
| } | |
| aapClient := aap.NewClient(aapURL, aapToken, insecureSkipVerify) | |
| var err error | |
| provisioningProvider, err = provisioning.NewProvider(provisioning.ProviderConfig{ | |
| if aapURL := os.Getenv(controller.EnvAAPURL); aapURL != "" { | |
| aapToken := os.Getenv(controller.EnvAAPToken) | |
| if aapToken == "" { | |
| setupLog.Error(nil, fmt.Sprintf("%s must be set when %s is configured", controller.EnvAAPToken, controller.EnvAAPURL)) | |
| os.Exit(1) | |
| } | |
| insecureSkipVerify, _ := strconv.ParseBool(os.Getenv(controller.EnvAAPInsecureSkipVerify)) | |
| templatePrefix := os.Getenv(controller.EnvAAPTemplatePrefix) | |
| if templatePrefix == "" { | |
| templatePrefix = "osac" | |
| } | |
| aapClient := aap.NewClient(aapURL, aapToken, insecureSkipVerify) | |
| var err error | |
| provisioningProvider, err = provisioning.NewProvider(provisioning.ProviderConfig{ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/main.go` around lines 238 - 247, When OSAC_AAP_URL (controller.EnvAAPURL)
is set but OSAC_AAP_TOKEN (controller.EnvAAPToken) is missing, modify the boot
logic to fail fast instead of constructing a broken aap client; check the value
of aapToken after reading it from the environment and return/log a fatal error
(stop startup) if it's empty. Update the block that constructs
aap.NewClient(...) and provisioning.NewProvider(...) so you validate aapToken
first and only call aap.NewClient and provisioning.NewProvider with a valid
token, ensuring the application exits with a clear error rather than continuing
with an invalid provisioning.ProviderConfig.
tzumainn
left a comment
tzumainn
left a comment
There was a problem hiding this comment.
Generally, this looks pretty good! Two quick questions:
- how does the AAP provider know to call
playbook_osac_create_host_lease.ymlandplaybook_osac_delete_host_lease.yml? - where are the templates (like
bm_host_image_provision) actually passed in?
| EnvAAPURL = "OSAC_AAP_URL" | ||
| EnvAAPToken = "OSAC_AAP_TOKEN" | ||
| EnvAAPInsecureSkipVerify = "OSAC_AAP_INSECURE_SKIP_VERIFY" | ||
| EnvAAPTemplatePrefix = "OSAC_AAP_TEMPLATE_PREFIX" |
There was a problem hiding this comment.
I'd move these into cmd/main.go to be consistent with the other OSAC operators. The reason is that these are not necessarily host lease specific values; in an operator with multiple controller, you'd still want to use these values..
| } | ||
|
|
||
| // Provisioning runs first — power reconciliation is suspended during provisioning | ||
| if r.ProvisioningProvider != nil && hostLease.Spec.TemplateID != "" && hostLease.Spec.TemplateID != "noop" { |
There was a problem hiding this comment.
This is absolutely my fault, but I don't know that noop is actually a good string to use here. Not something that needs to be fixed now, especially since it looks like noop is explicitly set by the bare-metal-fulfillment-operator
resolveTemplateName() takes the action
|
Got it - makes sense! |
fd2d163 to
6c65743
Compare
- Add provisioning lifecycle: trigger AAP provision jobs when templateID is set, poll for completion, and set ProvisionTemplateComplete condition - Add deprovisioning lifecycle: trigger AAP deprovision jobs on deletion before finalizer removal, with status flush to prevent duplicate jobs on crash - Refactor Reconcile into handleUpdate/reconcileDelete with centralized status updates Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6c65743 to
2af5276
Compare
2 participants
Summary
deprovisioning jobs for HostLease resources
Reconcile()instead of scatteringStatus().Update()calls across helpers
Changes
Provisioning lifecycle (
reconcileProvisioning)ProvisionTemplateCompletecondition to track progress/success/failuredesiredConfigVersionhash to detect spec drift and re-provisionDeprovisioning lifecycle (
reconcileDeprovisioning)DeprovisionTemplateCompleteconditionStatus management refactor
syncHostLeaseStatusnow mutates the in-memory object only (no longer callsStatus().Update())Reconcile()does a single Status().Update() at the end by comparing old vs new status withequality.Semantic.DeepEqualhandleUpdate/reconcileDeletefor clarityOther
OSAC_AAP_URL,OSAC_AAP_TOKEN,OSAC_AAP_INSECURE_SKIP_VERIFY,OSAC_AAP_TEMPLATE_PREFIX)bare-metal-operator->bare-metal-fulfillment-operator, bumpedosac-operator to v0.1.1